feat: transfer tx fees to treasury by Roznovjak · Pull Request #42 · galacticcouncil/warehouse

Roznovjak · 2022-04-06T08:23:13Z

No description provided.

codecov · 2022-04-07T00:54:05Z

Codecov Report

Merging #42 (4455be8) into main (c73846e) will decrease coverage by 0.59%.
The diff coverage is 87.01%.

@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
- Coverage   77.06%   76.46%   -0.60%     
==========================================
  Files          16       16              
  Lines         933      973      +40     
==========================================
+ Hits          719      744      +25     
- Misses        214      229      +15

Impacted Files	Coverage Δ
transaction-multi-payment/src/lib.rs	`65.76% <87.01%> (-0.91%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c73846e...4455be8. Read the comment docs.

Roznovjak · 2022-04-07T12:35:45Z

One notable change is that we replaced withdraw and deposit with one transfer. The consequence is that we can't use withdraw_reason, we don't transfer non-native assets with the KeepAlive parameter and we also don't use deposit_into_existing(). It's because existing currency adapters don't allow us to specify all "low level" parameters and use fixed values for some parameters.

enthusiastmartin · 2022-04-15T08:00:18Z

i would suggest to keep the original implementation where the native is withdrawn and refunds are deposited.

and introduce new one with fees transferred to selected account.

in this way - it will be easily to swap back if needed.

enthusiastmartin · 2022-05-07T12:38:16Z

@apopiak Could you have a look at this ?

the reason is that it seems that this PR introduces something similar to what you'd need for xcm trader ( see the TransactioPaymentDataProvider).
So perhaps, we can/should consolidate this into one thing.

Also, review of the PR by you would be appreciated too.

apopiak · 2022-05-09T14:05:38Z

transaction-multi-payment/src/lib.rs

+            match paid {
+                PaymentInfo::Native(paid_fee) => {
+                    let refund_amount = paid_fee.saturating_sub(corrected_fee);
+                    C::transfer(&fee_receiver, who, refund_amount, ExistenceRequirement::KeepAlive)


Doing a transfer up above in withdraw and then doing another one here means that the fees could have moved from the fee receiver account in the mean time so you won't be able to refund.
It's probably fine but if that edge case ever happens it'll be one hell of a thing to debug ;-)

Also: you might want to skip the transfer in case refund_amount is zero?

How else would be possible to withdraw fee and avoiding such scenarios? Would reserve some balance instead of transfer work ?

i mean, you'd first reserve and then only transfer the actual fee and unreserve the remaining balance ?

The MultiCurrency trait offers withdraw and deposit which could be used together similar to how Substrate's transaction payment is implremented

The problem is that Currency and MultiCurrency traits are inconsistent. Currency returns Imbalance type, MultiCurrency returns Balance type, and it's impossible to combine them. We can't create an imbalance from some balance and we also can't get rid of imbalance without calling it's drop implementation. This is the main reason why we are using transfers in our implementation. We can handle each currency type separately and use withdraw methods, but then in case of any error during execution of an extrinsic, tokens are burned instead of deposited because correct_and deposit_fee is not called. And I'm not sure if we want this behavior.

Is skipping the deposit in case refund is zero really needed? Both deposit methods (native and non-native) have a check for zero balance at the beginning.

🤷 good point, that's a fair assumption for the most common implementations.

transaction-multi-payment/src/lib.rs

Co-authored-by: Alexander Popiak <alexander.popiak@gmail.com>

transaction-multi-payment/src/lib.rs

mrq1911 · 2022-05-26T08:46:38Z

transaction-multi-payment/src/mock.rs


+pub struct DepositAll;
+
+impl DepositFee<AccountId, AssetId, Balance> for DepositAll {


@Roznovjak can you please provide this implementation in pallet (or mby even as default), so it doesn't have to be implemented in the runtime?

Would you like to have it implemented for the unit type () or just to have DepositFee struct publicly available from the pallet?

enthusiastmartin

Looks good.

we just need to make sure that no tokens are lost ( after the withdraw and deposit_fee).

transaction-multi-payment/src/lib.rs

transaction-multi-payment/Cargo.toml

enthusiastmartin · 2022-05-27T12:35:54Z

transaction-multi-payment/src/lib.rs

-    /// Account to use when pool does not exist.
-    #[pallet::storage]
-    #[pallet::getter(fn fallback_account)]
-    pub type FallbackAccount<T: Config> = StorageValue<_, T::AccountId, OptionQuery>;


No need for migration to delete the storage ? only if upgraded after it is already in use.

transaction-multi-payment/src/traits.rs

transaction-multi-payment/src/lib.rs

apopiak · 2022-05-27T16:47:29Z

you'll also want 1de7570 to allow Basilisk to compile

apopiak · 2022-05-28T09:42:22Z

and to add the feature "use_alloc" ce90275

apopiak · 2022-05-28T09:46:47Z

As you remove the MultiCurrencyAdapter this will need a companion PR for Basilisk, no?

Co-authored-by: Alexander Popiak <alexander.popiak@gmail.com>

… into tx_payment_fee

Roznovjak · 2022-05-30T19:40:04Z

As you remove the MultiCurrencyAdapter this will need a companion PR for Basilisk, no?

Yes. We will also need another PR for hydra-parachain.

jak-pan · 2022-06-13T10:49:29Z

Is this ready for merge?

enthusiastmartin · 2022-06-13T10:53:26Z

Is this ready for merge?

I would like to resolve first this question

It just looks to me bit unnecessary. i would simplify it. Let's wait for Richard to give his opinion.

Other than that - it is ready for merge.

apopiak · 2022-06-13T11:00:40Z

He removed the iterator

initial implementation

dba9965

Roznovjak requested a review from enthusiastmartin April 6, 2022 08:23

Roznovjak added 2 commits April 6, 2022 23:13

add tests

22f0310

remove unused stuff

9c2f3ff

Roznovjak marked this pull request as ready for review April 7, 2022 00:24

Roznovjak added 3 commits April 15, 2022 15:07

add previous implementation

56b7065

rename fallback_account to fee_receiver

abefdde

enable existential deposit in tests

995dc22

Roznovjak self-assigned this Apr 25, 2022

enthusiastmartin requested a review from apopiak May 7, 2022 12:38

apopiak reviewed May 9, 2022

View reviewed changes

transaction-multi-payment/src/lib.rs Outdated Show resolved Hide resolved

Roznovjak and others added 6 commits May 10, 2022 15:04

rename converted_fee to refund

07117da

Co-authored-by: Alexander Popiak <alexander.popiak@gmail.com>

replace transfers with withdraws/deposits

72221b0

Merge branch 'main' into tx_payment_fee

e4895fe

formatting

71236e4

add DepositFee trait

4d2be81

formatting

60259e1

Roznovjak requested a review from apopiak May 20, 2022 11:29

jak-pan reviewed May 20, 2022

View reviewed changes

transaction-multi-payment/src/lib.rs Outdated Show resolved Hide resolved

Roznovjak added 2 commits May 23, 2022 16:38

move fee_receiver from GenesisConfig to Config

328f86d

formatting

9f78c9c

Roznovjak requested review from jak-pan and mrq1911 May 25, 2022 12:12

jak-pan approved these changes May 25, 2022

View reviewed changes

mrq1911 reviewed May 26, 2022

View reviewed changes

Roznovjak added 2 commits May 27, 2022 13:30

move DepositAll from mock to pallet

84c1e10

formatting

0a3fe4b

enthusiastmartin reviewed May 27, 2022

View reviewed changes

apopiak reviewed May 27, 2022

View reviewed changes

transaction-multi-payment/src/lib.rs Show resolved Hide resolved

apopiak reviewed May 27, 2022

View reviewed changes

transaction-multi-payment/src/lib.rs Outdated Show resolved Hide resolved

apopiak mentioned this pull request May 27, 2022

feat: Add custom XCM Weight Trader #48

Merged

Roznovjak and others added 5 commits May 30, 2022 08:25

bump crate version

1d7f215

Update transaction-multi-payment/src/lib.rs

d8ed820

Co-authored-by: Alexander Popiak <alexander.popiak@gmail.com>

Merge branch 'tx_payment_fee' of github.com:galacticcouncil/warehouse…

9c64989

… into tx_payment_fee

various small adjustments

614fe2d

update DepositFee interface

d15eec6

Roznovjak added 2 commits June 3, 2022 15:12

Merge branch 'main' into tx_payment_fee

c2cd0ea

Merge branch 'main' into tx_payment_fee

4455be8

Roznovjak mentioned this pull request Jun 6, 2022

refund tx fee if paid in non-native currency galacticcouncil/Basilisk-node#166

Closed

apopiak approved these changes Jun 13, 2022

View reviewed changes

mrq1911 merged commit 50c82ab into main Jun 13, 2022

jak-pan mentioned this pull request Jun 13, 2022

Fee's for execution goes to Treasury galacticcouncil/Basilisk-node#412

Closed

Roznovjak deleted the tx_payment_fee branch June 13, 2022 14:01

apopiak mentioned this pull request Jun 14, 2022

feat!: Transfer Fees to Treasury galacticcouncil/Basilisk-node#493

Merged

3 tasks


		pub struct DepositAll;

		impl DepositFee<AccountId, AssetId, Balance> for DepositAll {

Conversation

Roznovjak commented Apr 6, 2022

Uh oh!

codecov bot commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Roznovjak commented Apr 7, 2022

Uh oh!

enthusiastmartin commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enthusiastmartin commented May 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mrq1911 May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enthusiastmartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apopiak commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apopiak commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apopiak commented May 28, 2022

Uh oh!

Roznovjak commented May 30, 2022

Uh oh!

jak-pan commented Jun 13, 2022

Uh oh!

enthusiastmartin commented Jun 13, 2022

Uh oh!

apopiak commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Apr 7, 2022 •

edited

Loading

enthusiastmartin commented Apr 15, 2022 •

edited

Loading

mrq1911 May 26, 2022 •

edited

Loading

apopiak commented May 27, 2022 •

edited

Loading

apopiak commented May 28, 2022 •

edited

Loading